feat: refractored SQL-based alerting framework #10605
Conversation
JasonD28
force-pushed
the
jdavis_alert_val
branch
from
88b560f to
9b76700
Compare
JasonD28
force-pushed
the
jdavis_alert_val
branch
from
25349a6 to
891288d
Compare
Codecov Report
@@ Coverage Diff @@
## master #10605 +/- ##
==========================================
+ Coverage 58.92% 60.35% +1.42%
==========================================
Files 756 363 -393
Lines 36072 23421 -12651
Branches 3301 0 -3301
==========================================
- Hits 21256 14135 -7121
+ Misses 14633 9286 -5347
+ Partials 183 0 -183
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
JasonD28
force-pushed
the
jdavis_alert_val
branch
from
36d7ca4 to
2c6dc25
Compare
JasonD28
changed the title
bkyryliuk
changed the title
superset/models/alerts.py
Outdated
| backref=backref("sql_observers", cascade="all, delete-orphan"), | ||
| ) | ||
|
|
||
| # TODO: Abstract observations from the sqlamodls e.g. |
There was a problem hiding this comment.
s/sqlamodls/sqlalchemy models
add some context, e.g. observation table will be constantly growing and would need to either have some retention policy or be moved to the more scalable db. Same as alert log
superset/models/alerts.py
Outdated
| default=textwrap.dedent( | ||
| """ | ||
| { | ||
| "example_threshold": 50 |
superset/models/alerts.py
Outdated
| ) | ||
|
|
||
|
|
||
| def not_null_executor( |
There was a problem hiding this comment.
this should live outside the models, you can create a separate module for them
e.g. tasks/schedules/alerts/validator.py
superset/models/alerts.py
Outdated
| """Returns True if a SQLObserver's recent observation is not NULL""" | ||
|
|
||
| observation = observer.get_observations(1)[0] | ||
| if observation.value: |
There was a problem hiding this comment.
this one could be tricky
if 0 returns false :)
superset/tasks/schedules.py
Outdated
| @@ -538,16 +543,15 @@ def schedule_alert_query( # pylint: disable=unused-argument | |||
| logger.info("Ignoring deactivated alert") | |||
| return | |||
|
|
|||
| if schedule.sql_observers: | |||
| sql = schedule.sql_observers[0].sql | |||
There was a problem hiding this comment.
could there be multiple observers?
if not, let's change sql_observers to sql_observer
tests/alerts_tests.py
Outdated
| @@ -41,95 +50,204 @@ | |||
| def setup_database(): | |||
| with app.app_context(): | |||
| slice_id = db.session.query(Slice).all()[0].id | |||
| database_id = utils.get_example_database().id | |||
| database = utils.get_example_database() | |||
tests/alerts_tests.py
Outdated
| alert_type="email", | ||
| slice_id=slice_id, | ||
| database_id=database_id, | ||
| Alert(**common_data, id=1, label="alert_1"), |
There was a problem hiding this comment.
let's not specify ids here and those are autogenerated, use label to retrieve alerts you care about
There was a problem hiding this comment.
I specified id's in order to control with SQLObserver is linked to which alert (alert_id column in SQLObserver), I'm not sure if there is another way to do this
tests/alerts_tests.py
Outdated
| # Test error with alert lacking observer | ||
| alert5 = dbsession.query(Alert).filter_by(id=5).one() | ||
| check_alert(alert5.id, alert5.label) | ||
| assert alert5.logs[-1].state == AlertState.ERROR |
There was a problem hiding this comment.
let's add successful alert tests as well
tests/alerts_tests.py
Outdated
| dbsession = setup_database | ||
| null_val1 = Validator( |
There was a problem hiding this comment.
let's decouple db and validators, validators are just the simple functions.
You can pass observation list and config to them instead.
|
@willbarrett when you have a chance could you take a look? |
JasonD28
force-pushed
the
jdavis_alert_val
branch
from
b4da293 to
7b94710
Compare
|
@bkyryliuk the observer/validator pattern is really confusing, I heard there's been discussions on reverting this. Personally I feel like we should 100% revert this. What's the plan!? |
@mistercrunch we have synced with @willbarrett on this topic. I will merge the observer & validator back into alert object within next 2 weeks. Will keep Observation object to support validation across multiple observations. |
|
Thanks for the response @bkyryliuk - if you're unable to get to it this week please let me know and we'll take on the task on our side. We're under some time pressure to evolve this feature. Thanks! |
* added new tables for alerting refractor * reformatted inheritance structure * added workflow for updated framework * added suggested changes * cleaned up changes * added obervations to alert table to enable view * added comments * added requested changes * fix tests * added styling changes * mypy * added requested changes * updated operator logic * requested changes, 1 validator, styling changes * refactored tests * fix test alert workflow * fixed create_alert in test Co-authored-by: Jason Davis <@dropbox.com>
mistercrunch
added
🏷️ bot
SUMMARY
Currently superset has all of its functionality centralized inside the Alerts table. This PR separates the different functionalities of alerting into separate components in order to increase the functionality of alerting and to allow this feature more flexibility for future upgrades.
Major Changes:
SQLObserverswhich will handle the functionality of making SQL queries for the alertSQLObservationswhich will hold the information gathered from each SQLObserver queryValidatorswhich will handle looking at the values fromSQLObservationsto determine if an alert should be triggeredBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TEST PLAN
ADDITIONAL INFORMATION